Skip to content

Simplify doc meta mode - #159473

Open
notriddle wants to merge 12 commits into
rust-lang:mainfrom
notriddle:rename-parts-to-dep-meta
Open

Simplify doc meta mode#159473
notriddle wants to merge 12 commits into
rust-lang:mainfrom
notriddle:rename-parts-to-dep-meta

Conversation

@notriddle

@notriddle notriddle commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

View all comments

Follow up #159415 (comment)

Get rid of the mode where you can finalize the CCI and generate more docs at the same time. It isn't used in Cargo, and probably won't be used elsewhere?

@rustbot rustbot added A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. labels Jul 17, 2026
@rustbot

rustbot commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

r? @folkertdev

rustbot has assigned @folkertdev.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Why was this reviewer chosen?

The reviewer was selected based on:

  • Owners of files modified in this PR: compiler
  • compiler expanded to 74 candidates
  • Random selection from 17 candidates

@notriddle

Copy link
Copy Markdown
Contributor Author

r? @camelid

@rustbot rustbot assigned camelid and unassigned folkertdev Jul 17, 2026
@rustbot

rustbot commented Jul 17, 2026

Copy link
Copy Markdown
Collaborator

camelid is currently at their maximum review capacity.
They may take a while to respond.

Comment thread src/librustdoc/html/render/write_shared.rs
Comment thread tests/run-make/rustdoc-merge-cross-crate-info-kitchen-sink-separate-dirs/rmake.rs Outdated
Comment thread src/librustdoc/lib.rs
Comment thread src/doc/rustdoc/src/unstable-features.md Outdated
Comment thread src/doc/rustdoc/src/unstable-features.md Outdated
@camelid camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 23, 2026
notriddle and others added 9 commits July 25, 2026 19:06
Since we're getting rid of the ability to do finalize and document at
the same time, we can't actually do this in the rustdoc-html test build
system and need to do it this way instead.
Co-authored-by: Noah Lev <37223377+camelid@users.noreply.github.com>
Co-authored-by: Noah Lev <37223377+camelid@users.noreply.github.com>
@notriddle
notriddle force-pushed the rename-parts-to-dep-meta branch from 9e2125b to d0fec14 Compare July 26, 2026 02:19
@rustbot

rustbot commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 27, 2026

@camelid camelid left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall, but I'd like to avoid the dummy crate and TyCtxt, and some of the tests seem to be translated wrong from the original files.

View changes since this review

Comment thread src/librustdoc/lib.rs Outdated
Comment on lines +790 to +796
let krate = ast::Crate {
attrs: Default::default(),
items: Default::default(),
spans: Default::default(),
id: ast::DUMMY_NODE_ID,
is_placeholder: false,
};

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love that we're creating a dummy crate here just to get a TyCtxt. It seems like we only use the TyCtxt to get the Sess. So I don't think you actually need to do this. interface::Compiler already has a Sess on it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've removed the synthetic crate, but not the synthetic InputStr.

Comment thread src/librustdoc/lib.rs
Comment on lines +861 to +872
let config = core::create_config(
Input::Str {
name: rustc_span::FileName::Custom(String::new()),
input: String::new(),
},
options,
&render_options,
);
return wrap_return(
dcx,
rustc_span::create_session_globals_then(options.edition, &[], None, || {
run_merge_finalize(render_options)
interface::run_compiler(config, |compiler| {
run_merge_finalize(render_options, compiler)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto here, but getting rid of the fake Input::Str is harder, and this seems less bad than making a fake crate and TyCtxt. So fine to just leave it.

.arg(format!("--read-doc-meta-dir={}", parts_out_dir.display()))
.arg("--enable-index-page")
.run();
output.assert_stderr_not_contains("error: the compiler unexpectedly panicked. this is a bug.");

@camelid camelid Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Is this necessary? Shouldn't the run-make test fail automatically if one of its subprocesses ICEs? (I'm not super familiar with run-make, so I might be wrong about this.)

@notriddle notriddle Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked, and you're right.

pub fn run(&mut self) -> CompletedProcess {
let output = self.command_output();
if !output.status().success() {
handle_failed_output(&self, output, panic::Location::caller().line());
} else {
verbose_print_command(self, &output);
}
output
}

Apparently, when a run-make command exits with a non-zero exit code, this thing will fail the test.

pub(crate) fn handle_failed_output(
cmd: &Command,
output: CompletedProcess,
caller_line_number: u32,
) -> ! {
if output.status().success() {
eprintln!("command unexpectedly succeeded at line {caller_line_number}");
} else {
eprintln!("command failed at line {caller_line_number}");
}
print_command_output(cmd, &output);
std::process::exit(1)
}


fn main() {
let merged_dir = path("merged");
let parts_out_dir = path("parts");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this just not used at all?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not. I've removed it.

.library_search_path(cwd())
.out_dir(&out_dir)
.arg("-Zunstable-options")
.arg("--write-doc-meta-dir=parts-wrong")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this is parts-wrong -- if they all use the same name, there's nothing wrong about it. I think this might have been lost in translation from the original test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no --read-doc-meta-dir, and the sierra.rs tests all verify that the shared resources aren't generated.

I've added a comment, and renamed the dir parts-unused, to clarify this.

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 27, 2026
@rustbot

rustbot commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rust-log-analyzer

This comment has been minimized.

@notriddle
notriddle force-pushed the rename-parts-to-dep-meta branch from c3d3011 to f73cd1e Compare July 28, 2026 04:05
If this parameter is supplied but `--read-doc-meta-dir` isn't, it runs in *intermediate mode*:
some pages may be written to the output dir, but there is a lot of functionality that won't work
until rustdoc is run in *finalize mode*.
When `--write-doc-meta-dir` is supplied, rustdoc will write the crate's shared metadata to

@weihanglo weihanglo Jul 29, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably not the right question for this PR, and I haven't read the implementation here, though I am curious:

  • How does this interact with --out-dir. I guess --out-dir is ignored here?
  • How does this interact with all those --emit files (w/ or w/o --out-dir? Where will those files be emitted?

View changes since the review

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Context: I'd like to pass --emit=dep-info without specifying the full (absolute) path for those dep-info files. Because of this issue #159743 we cannot specify full path, and currently blocks rust-lang/cargo#17020.

(That said, it is a very Cargo specific issue I feel like)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The --out-dir is still used as the path for --emit files. --write-doc-meta-dir doesn't pay attention to --out-dir.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. So eventually if individual rustdoc doesn't need to emit html contents, cargo then can use --emit to control dep-info emission location exclusively. Thanks!

@camelid

camelid commented Jul 29, 2026

Copy link
Copy Markdown
Member

We're also missing the root-level help.html and settings.html files in the finalize step.

@notriddle

Copy link
Copy Markdown
Contributor Author

Good catch! settings.html and help.html are moved, so they get generated at finalize now.

@rust-log-analyzer

This comment has been minimized.

@notriddle
notriddle force-pushed the rename-parts-to-dep-meta branch from 23fae6c to 5f940df Compare July 30, 2026 05:57

@camelid camelid left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! r=me with comments addressed. Or feel free to just merge and then fix in a later PR since this is already a big improvement.

Also, could you rename the title and edit the PR description since the scope has changed and grown a bit?

View changes since this review

}),
&style_files,
);
let mut file = try_err!(File::create_buffered(&settings_file), &settings_file);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I think it would actually be more efficient not to use a buffer since we're writing the whole thing in one go. We can just use std::fs::write probably. Ditto with the other write below.

//@ has index.html
//@ has index.html '//h1' 'List of all crates'
//@ has index.html '//ul[@class="all-items"]//a[@href="quebec/index.html"]' 'quebec'
//@ has index.html '//ul[@class="all-items"]//a[@href="sierra/index.html"]' 'sierra'
//@ has index.html '//ul[@class="all-items"]//a[@href="tango/index.html"]' 'tango'
//@ has help.html
//@ has settings.html

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was going to ask for tests for this, but you were two steps ahead. :)

Comment on lines 666 to 669
let mut root_path = self.dst.to_str().expect("invalid path").to_owned();
if !root_path.ends_with('/') {
root_path.push('/');
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT this is all actually dead code.

Comment on lines +173 to +176
let mut root_path = dst.to_str().expect("invalid path").to_owned();
if !root_path.ends_with('/') {
root_path.push('/');
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

Comment thread src/librustdoc/lib.rs
Comment on lines +799 to +800
logo: String::new(),
favicon: String::new(),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like these (and scrape_examples_extension) should be filled in with real values.

@camelid

camelid commented Jul 31, 2026

Copy link
Copy Markdown
Member

Have we specified the behavior of rustdoc when two different invocations use the same --write-doc-meta-dir folder?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-run-make Area: port run-make Makefiles to rmake.rs S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants